Extend MemoryEffects to Support Target-Specific Memory Locations #148650
Conversation
llvm/include/llvm/Support/ModRef.h
Outdated
There was a problem hiding this comment.
Each Target should have an InaccessibleTargetMemLocation, but I am not sure if we should check which Target it is being used in Support, Attributes and so on. I would appreciate some suggestion if this is the right way to do this.
And this is still using Data to model the Memory Locations with ModRef. Is this the only way to add new memory location?
There was a problem hiding this comment.
I wonder if we could have something around the following instead:
enum class IRMemLocation {
ArgMem = 0,
InaccessibleMem = 1,
ErrnoMem = 2,
TargetSpecificMem = 3,
Other = 4,
First = ArgMem,
Last = Other,
};And then extending MemoryEffectsBase to have, like, a Optional<TargetSpecificMemTag> TargetMem; field, where TargetMem is, e.g.,:
struct TargetSpecificMemTag {
Triple::ArchType Arch;
uint8_t Kind;
};This would lead to know easily if the location is target-specific:
bool isTargetSpecific() const { return Loc == Location::TargetSpecificMem; }And I guess it would avoid having some static_cast<IRMemLocation> around. Not sure if the target-specific memory effects are decoupled enough this way though.
There was a problem hiding this comment.
Hi @antoniofrighetto
I will try to follow your suggestion in a next iteration. I am not sure I fully understand the consequences of implementing this way.
For instance: I am not sure I can have only one Kind.
For instance, something like this:
def int_aarch64_get_fpmr_set_za : DefaultAttrsIntrinsic<[], [llvm_i64_ty], [IntrInaccessibleReadMemOnly<AArch64_FPMR>, IntrInaccessibleWriteMemOnly<AArch64_ZA>]>;
I imagine that the ModRefInfo for TargetSpecificMem will be 11, that is ModRef.
But how Kind will work in the case above? Do you have an idea? Because we need to set one to just write and another to just read.
I can only think about another TargetMemoryLocation and another vector like Data, but only used when TargetSpecificMem !=NoModRef
I would also have to see how this will work in BasicAliasAnalysis.cpp/getModRefInfo and BitcodeReader.cpp(parseAttributeGroupBlock)
There was a problem hiding this comment.
Hi @antoniofrighetto,
I think your suggestion need me to change the way Memory Location is implemented.
However I did follow @nikic's solution(or I think I did) and create a generic name for the target memory locations, and they are printed according to Triple. I think this makes It generic enough.
Let me know what you think about the latest change.
I have some follow up patches :
#154141
#154144
#155590
that I will rebase once this patch is merged.
|
Making things target-specific at this level is not worthwhile. Similar to how intrinsics are always defined, regardless of whether the corresponding target was built, these can also just always be available, but will only be used for specific targets. |
CarolineConcatto
force-pushed
the
fpmr_metadata
branch
2 times, most recently
from
7512b21 to
13c7470
Compare
|
Hi @nikic , Thank you for your comments, I do have some extra notes. My concern is that with Data being 32 bits and reserving 2 bits for each location, there are 16 memory locations available. There are 3 already being reserved to ArgMem, InaccessibleMem and Other. Now AArch64 is using 2(AArch64_FPRM and AArch64_ZA, I will probably expand to use for ZT0), if each target adds its own location it could be out of space with Data. Any suggestion how to implement the Target Memory Location? The way it is implemented the Target Memory Locations for now is correct ? Should I change? Another question, should we treat these extra locations as Inaccessible memory as well? |
|
@llvm/pr-subscribers-tablegen @llvm/pr-subscribers-llvm-support Author: None (CarolineConcatto) Changes…ocations This patch introduces preliminary support for additional memory locations, such as FPMR and ZA, needed to model AArch64 architectural registers as memory dependencies. It was a solution suggested in : https://discourse.llvm.org/t/rfc-improving-fpmr-handling-for-fp8-intrinsics-in-llvm/86868/6 Currently, these locations are not yet target-specific. The goal is to enable the compiler to express read/write effects on these resources. What This Patch Does: Current Limitations: This patch is not functionally complete — it is a structural prototype to solicit feedback on the direction and I would like some suggestion on how to proceed. Full diff: https://github.com/llvm/llvm-project/pull/148650.diff 15 Files Affected:
diff --git a/llvm/include/llvm/AsmParser/LLToken.h b/llvm/include/llvm/AsmParser/LLToken.h
index c7e4bdf3ff811..c08eb99c1f5b2 100644
--- a/llvm/include/llvm/AsmParser/LLToken.h
+++ b/llvm/include/llvm/AsmParser/LLToken.h
@@ -202,6 +202,8 @@ enum Kind {
kw_readwrite,
kw_argmem,
kw_inaccessiblemem,
+ kw_aarch64_fpmr,
+ kw_aarch64_za,
kw_errnomem,
// Legacy attributes:
diff --git a/llvm/include/llvm/IR/Intrinsics.td b/llvm/include/llvm/IR/Intrinsics.td
index bd6f94ac1286c..ad1b0b462be37 100644
--- a/llvm/include/llvm/IR/Intrinsics.td
+++ b/llvm/include/llvm/IR/Intrinsics.td
@@ -49,6 +49,17 @@ def IntrArgMemOnly : IntrinsicProperty;
// accessible by the module being compiled. This is a weaker form of IntrNoMem.
def IntrInaccessibleMemOnly : IntrinsicProperty;
+
+
+class IntrinsicMemoryLocation;
+// This should be added in the Target, but once in IntrinsicsAArch64.td
+// It complains error: "Variable not defined: 'AArch64_FPMR'"
+def AArch64_FPMR : IntrinsicMemoryLocation;
+def AArch64_ZA: IntrinsicMemoryLocation;
+// IntrInaccessible{Read|Write}MemOnly needs to set Location
+class IntrInaccessibleReadMemOnly<IntrinsicMemoryLocation idx> : IntrinsicProperty{IntrinsicMemoryLocation Loc=idx;}
+class IntrInaccessibleWriteMemOnly<IntrinsicMemoryLocation idx> : IntrinsicProperty{IntrinsicMemoryLocation Loc=idx;}
+
// IntrInaccessibleMemOrArgMemOnly -- This intrinsic only accesses memory that
// its pointer-typed arguments point to or memory that is not accessible
// by the module being compiled. This is a weaker form of IntrArgMemOnly.
diff --git a/llvm/include/llvm/Support/ModRef.h b/llvm/include/llvm/Support/ModRef.h
index 71f3b5bcb9c2b..0de2b02e4e05a 100644
--- a/llvm/include/llvm/Support/ModRef.h
+++ b/llvm/include/llvm/Support/ModRef.h
@@ -56,6 +56,11 @@ enum class ModRefInfo : uint8_t {
/// Debug print ModRefInfo.
LLVM_ABI raw_ostream &operator<<(raw_ostream &OS, ModRefInfo MR);
+enum class InaccessibleTargetMemLocation {
+ AARCH64_FPMR = 3,
+ AARCH64_ZA = 4,
+};
+
/// The locations at which a function might access memory.
enum class IRMemLocation {
/// Access to memory via argument pointers.
@@ -65,7 +70,7 @@ enum class IRMemLocation {
/// Errno memory.
ErrnoMem = 2,
/// Any other memory.
- Other = 3,
+ Other = 5,
/// Helpers to iterate all locations in the MemoryEffectsBase class.
First = ArgMem,
@@ -152,6 +157,40 @@ template <typename LocationEnum> class MemoryEffectsBase {
return MemoryEffectsBase(Location::Other, MR);
}
+ /// Create MemoryEffectsBase that can only read inaccessible memory.
+ static MemoryEffectsBase
+ inaccessibleReadMemOnly(Location Loc = Location::InaccessibleMem) {
+ return MemoryEffectsBase(Loc, ModRefInfo::Ref);
+ }
+
+ /// Create MemoryEffectsBase that can only write inaccessible memory.
+ static MemoryEffectsBase
+ inaccessibleWriteMemOnly(Location Loc = Location::InaccessibleMem) {
+ return MemoryEffectsBase(Loc, ModRefInfo::Mod);
+ }
+
+ /// Checks if only target-specific memory locations are set.
+ /// Ignores standard locations like ArgMem or InaccessibleMem.
+ /// Needed because `Data` may be non-zero by default unless explicitly
+ /// cleared.
+ bool onlyAccessTargetMemoryLocation() {
+ MemoryEffectsBase ME = *this;
+ for (unsigned I = static_cast<int>(LocationEnum::ErrnoMem);
+ I < static_cast<int>(LocationEnum::Last); I++)
+ ME = ME.getWithoutLoc(static_cast<IRMemLocation>(I));
+ return ME.doesNotAccessMemory();
+ }
+
+ /// Create MemoryEffectsBase that can only access Target Memory Locations
+ static MemoryEffectsBase
+ setTargetMemLocationModRef(ModRefInfo MR = ModRefInfo::NoModRef) {
+ MemoryEffectsBase FRMB = none();
+ for (unsigned I = static_cast<int>(LocationEnum::ErrnoMem);
+ I < static_cast<int>(LocationEnum::Last); I++)
+ FRMB.setModRef(static_cast<Location>(I), MR);
+ return FRMB;
+ }
+
/// Create MemoryEffectsBase that can only access inaccessible or argument
/// memory.
static MemoryEffectsBase
@@ -178,6 +217,11 @@ template <typename LocationEnum> class MemoryEffectsBase {
return MemoryEffectsBase(Data);
}
+ bool isTargetMemLoc(IRMemLocation Loc) {
+ return static_cast<unsigned>(Loc) >
+ static_cast<unsigned>(Location::ErrnoMem);
+ }
+
/// Convert MemoryEffectsBase into an encoded integer value (used by memory
/// attribute).
uint32_t toIntValue() const {
diff --git a/llvm/include/llvm/TableGen/Record.h b/llvm/include/llvm/TableGen/Record.h
index a2b86eb8e7cad..5aeb331c49c9b 100644
--- a/llvm/include/llvm/TableGen/Record.h
+++ b/llvm/include/llvm/TableGen/Record.h
@@ -25,6 +25,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/ModRef.h"
#include "llvm/Support/SMLoc.h"
#include "llvm/Support/Timer.h"
#include "llvm/Support/TrailingObjects.h"
@@ -1961,6 +1962,8 @@ class Record {
/// value is not the right type.
int64_t getValueAsInt(StringRef FieldName) const;
+ llvm::IRMemLocation getLocationTypeAsInt(StringRef FieldName) const;
+
/// This method looks up the specified field and returns its value as an Dag,
/// throwing an exception if the field does not exist or if the value is not
/// the right type.
diff --git a/llvm/lib/AsmParser/LLLexer.cpp b/llvm/lib/AsmParser/LLLexer.cpp
index ce813e1d7b1c4..c086f9f9585a2 100644
--- a/llvm/lib/AsmParser/LLLexer.cpp
+++ b/llvm/lib/AsmParser/LLLexer.cpp
@@ -701,6 +701,8 @@ lltok::Kind LLLexer::LexIdentifier() {
KEYWORD(write);
KEYWORD(readwrite);
KEYWORD(argmem);
+ KEYWORD(aarch64_fpmr);
+ KEYWORD(aarch64_za);
KEYWORD(inaccessiblemem);
KEYWORD(errnomem);
KEYWORD(argmemonly);
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index b7f6950f679ef..abde2993bb048 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -1666,6 +1666,25 @@ static bool upgradeMemoryAttr(MemoryEffects &ME, lltok::Kind Kind) {
}
}
+static std::optional<MemoryEffects::Location> keywordToLoc(lltok::Kind Tok) {
+ switch (Tok) {
+ case lltok::kw_argmem:
+ return IRMemLocation::ArgMem;
+ case lltok::kw_inaccessiblemem:
+ return IRMemLocation::InaccessibleMem;
+ case lltok::kw_errnomem:
+ return IRMemLocation::ErrnoMem;
+ case lltok::kw_aarch64_fpmr:
+ return static_cast<IRMemLocation>(
+ llvm::InaccessibleTargetMemLocation::AARCH64_FPMR);
+ case lltok::kw_aarch64_za:
+ return static_cast<IRMemLocation>(
+ llvm::InaccessibleTargetMemLocation::AARCH64_ZA);
+ default:
+ return std::nullopt;
+ }
+}
+
/// parseFnAttributeValuePairs
/// ::= <attr> | <attr> '=' <value>
bool LLParser::parseFnAttributeValuePairs(AttrBuilder &B,
@@ -2510,19 +2529,6 @@ bool LLParser::parseAllocKind(AllocFnKind &Kind) {
return false;
}
-static std::optional<MemoryEffects::Location> keywordToLoc(lltok::Kind Tok) {
- switch (Tok) {
- case lltok::kw_argmem:
- return IRMemLocation::ArgMem;
- case lltok::kw_inaccessiblemem:
- return IRMemLocation::InaccessibleMem;
- case lltok::kw_errnomem:
- return IRMemLocation::ErrnoMem;
- default:
- return std::nullopt;
- }
-}
-
static std::optional<ModRefInfo> keywordToModRef(lltok::Kind Tok) {
switch (Tok) {
case lltok::kw_none:
diff --git a/llvm/lib/IR/Attributes.cpp b/llvm/lib/IR/Attributes.cpp
index d1fbcb9e893a7..37e9d7c5c74db 100644
--- a/llvm/lib/IR/Attributes.cpp
+++ b/llvm/lib/IR/Attributes.cpp
@@ -640,6 +640,10 @@ std::string Attribute::getAsString(bool InAttrGrp) const {
if (MR == OtherMR)
continue;
+ // Dont want to print Target Location if NoModRef
+ if (ME.isTargetMemLoc(Loc) && (MR == ModRefInfo::NoModRef))
+ continue;
+
if (!First)
OS << ", ";
First = false;
@@ -656,6 +660,15 @@ std::string Attribute::getAsString(bool InAttrGrp) const {
break;
case IRMemLocation::Other:
llvm_unreachable("This is represented as the default access kind");
+ default: {
+ InaccessibleTargetMemLocation TargetLoc =
+ static_cast<InaccessibleTargetMemLocation>(Loc);
+ if (TargetLoc == InaccessibleTargetMemLocation::AARCH64_FPMR)
+ OS << "aarch64_fpmr: ";
+ if (TargetLoc == InaccessibleTargetMemLocation::AARCH64_ZA)
+ OS << "aarch64_za: ";
+ break;
+ }
}
OS << getModRefStr(MR);
}
diff --git a/llvm/lib/Support/ModRef.cpp b/llvm/lib/Support/ModRef.cpp
index 2bb9bc945bd2e..dc0dafdbe7e49 100644
--- a/llvm/lib/Support/ModRef.cpp
+++ b/llvm/lib/Support/ModRef.cpp
@@ -49,6 +49,15 @@ raw_ostream &llvm::operator<<(raw_ostream &OS, MemoryEffects ME) {
case IRMemLocation::Other:
OS << "Other: ";
break;
+ default: {
+ InaccessibleTargetMemLocation TargetLoc =
+ static_cast<InaccessibleTargetMemLocation>(Loc);
+ if (TargetLoc == InaccessibleTargetMemLocation::AARCH64_FPMR)
+ OS << "AARCH64_FPMR: ";
+ if (TargetLoc == InaccessibleTargetMemLocation::AARCH64_ZA)
+ OS << "AARCH64_ZA: ";
+ break;
+ }
}
OS << ME.getModRef(Loc);
});
diff --git a/llvm/lib/TableGen/Record.cpp b/llvm/lib/TableGen/Record.cpp
index 1f3e5dc68f1d6..d114358266737 100644
--- a/llvm/lib/TableGen/Record.cpp
+++ b/llvm/lib/TableGen/Record.cpp
@@ -3102,6 +3102,21 @@ Record::getValueAsListOfDefs(StringRef FieldName) const {
return Defs;
}
+llvm::IRMemLocation Record::getLocationTypeAsInt(StringRef FieldName) const {
+ const Record *LocRec = getValueAsDef(FieldName);
+ StringRef Name = LocRec->getName();
+ if (Name == "AArch64_FPMR")
+ return static_cast<IRMemLocation>(
+ llvm::InaccessibleTargetMemLocation::AARCH64_FPMR);
+ else if (Name == "AArch64_ZA")
+ return static_cast<IRMemLocation>(
+ llvm::InaccessibleTargetMemLocation::AARCH64_ZA);
+ else if (Name == "InaccessibleMem")
+ return llvm::IRMemLocation::InaccessibleMem;
+ else
+ PrintFatalError(getLoc(), "unknown IRMemLocation: " + Name);
+}
+
int64_t Record::getValueAsInt(StringRef FieldName) const {
const RecordVal *R = getValue(FieldName);
if (!R || !R->getValue())
diff --git a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
index f43202eea6306..49b822b3ef38e 100644
--- a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
@@ -143,6 +143,9 @@ static void addLocAccess(MemoryEffects &ME, const MemoryLocation &Loc,
ME |= MemoryEffects::argMemOnly(MR);
ME |= MemoryEffects(IRMemLocation::ErrnoMem, MR);
ME |= MemoryEffects(IRMemLocation::Other, MR);
+ // Should also set the other Target Memory Locations as MR.
+ // To compares with MemoryEffects::unknown() in addMemoryAttrs
+ ME |= MemoryEffects::setTargetMemLocationModRef(MR);
}
static void addArgLocs(MemoryEffects &ME, const CallBase *Call,
diff --git a/llvm/test/Assembler/memory-attribute.ll b/llvm/test/Assembler/memory-attribute.ll
index effd4ce7c4548..42f9b9f87e8b0 100644
--- a/llvm/test/Assembler/memory-attribute.ll
+++ b/llvm/test/Assembler/memory-attribute.ll
@@ -78,3 +78,28 @@ declare void @fn_argmem_read_inaccessiblemem_write()
; CHECK: @fn_argmem_read_inaccessiblemem_write_reordered()
declare void @fn_argmem_read_inaccessiblemem_write_reordered()
memory(inaccessiblemem: write, argmem: read)
+
+; CHECK: Function Attrs: memory(aarch64_za: write)
+; CHECK: @fn_inaccessiblemem_write_aarch64_za()
+declare void @fn_inaccessiblemem_write_aarch64_za()
+ memory(aarch64_za: write)
+
+; CHECK: Function Attrs: memory(aarch64_za: read)
+; CHECK: @fn_inaccessiblemem_read_aarch64_za()
+declare void @fn_inaccessiblemem_read_aarch64_za()
+ memory(aarch64_za: read)
+
+; CHECK: Function Attrs: memory(aarch64_fpmr: write)
+; CHECK: @fn_inaccessiblemem_write_aarch64_fpmr()
+declare void @fn_inaccessiblemem_write_aarch64_fpmr()
+ memory(aarch64_fpmr: write)
+
+; CHECK: Function Attrs: memory(aarch64_fpmr: read)
+; CHECK: @fn_inaccessiblemem_read_aarch64_fpmr()
+declare void @fn_inaccessiblemem_read_aarch64_fpmr()
+ memory(aarch64_fpmr: read)
+
+; CHECK: Function Attrs: memory(aarch64_fpmr: read, aarch64_za: write)
+; CHECK: @fn_inaccessiblemem_read_aarch64_fpmr_write_aarch64_za()
+declare void @fn_inaccessiblemem_read_aarch64_fpmr_write_aarch64_za()
+ memory(aarch64_fpmr: read, aarch64_za: write)
diff --git a/llvm/test/Bitcode/attributes.ll b/llvm/test/Bitcode/attributes.ll
index 8c1a76365e1b4..8e72e7ade54c1 100644
--- a/llvm/test/Bitcode/attributes.ll
+++ b/llvm/test/Bitcode/attributes.ll
@@ -572,7 +572,6 @@ define void @dead_on_return(ptr dead_on_return %p) {
ret void
}
-; CHECK: attributes #0 = { noreturn }
; CHECK: attributes #1 = { nounwind }
; CHECK: attributes #2 = { memory(none) }
; CHECK: attributes #3 = { memory(read) }
diff --git a/llvm/test/TableGen/intrinsic-attrs-fp8.td b/llvm/test/TableGen/intrinsic-attrs-fp8.td
new file mode 100644
index 0000000000000..5aaba44edcc45
--- /dev/null
+++ b/llvm/test/TableGen/intrinsic-attrs-fp8.td
@@ -0,0 +1,54 @@
+// RUN: llvm-tblgen -gen-intrinsic-impl -I %p/../../include -DTEST_INTRINSICS_SUPPRESS_DEFS %s | FileCheck %s
+
+include "llvm/IR/Intrinsics.td"
+
+def int_aarch64_set_fpmr_2 : DefaultAttrsIntrinsic<[], [llvm_i64_ty], [IntrInaccessibleWriteMemOnly<AArch64_FPMR>]>;
+
+def int_aarch64_get_za_2 : DefaultAttrsIntrinsic<[], [llvm_i64_ty], [IntrInaccessibleReadMemOnly<AArch64_ZA>]>;
+
+def int_aarch64_get_fpmr_set_za : DefaultAttrsIntrinsic<[], [llvm_i64_ty], [IntrInaccessibleReadMemOnly<AArch64_FPMR>, IntrInaccessibleWriteMemOnly<AArch64_ZA>]>;
+
+// CHECK: static constexpr unsigned IntrinsicNameOffsetTable[] = {
+// CHECK-NEXT: 1, // not_intrinsic
+// CHECK-NEXT: 15, // llvm.aarch64.get.fpmr.set.za
+// CHECK-NEXT: 44, // llvm.aarch64.get.za.2
+// CHECK-NEXT: 66, // llvm.aarch64.set.fpmr.2
+
+// CHECK: static AttributeSet getIntrinsicFnAttributeSet(LLVMContext &C, unsigned ID) {
+// CHECK-NEXT: switch (ID) {
+// CHECK-NEXT: default: llvm_unreachable("Invalid attribute set number");
+// CHECK-NEXT: case 0:
+// CHECK-NEXT: return AttributeSet::get(C, {
+// CHECK-NEXT: Attribute::get(C, Attribute::NoUnwind),
+// CHECK-NEXT: Attribute::get(C, Attribute::NoCallback),
+// CHECK-NEXT: Attribute::get(C, Attribute::NoSync),
+// CHECK-NEXT: Attribute::get(C, Attribute::NoFree),
+// CHECK-NEXT: Attribute::get(C, Attribute::WillReturn),
+// CHECK-NEXT: // ArgMem: NoModRef, InaccessibleMem: NoModRef, ErrnoMem: NoModRef, AARCH64_FPMR: Ref, AARCH64_ZA: Mod, Other: NoModRef
+// CHECK-NEXT: Attribute::getWithMemoryEffects(C, MemoryEffects::createFromIntValue(576)),
+// CHECK-NEXT: });
+// CHECK-NEXT: case 1:
+// CHECK-NEXT: return AttributeSet::get(C, {
+// CHECK-NEXT: Attribute::get(C, Attribute::NoUnwind),
+// CHECK-NEXT: Attribute::get(C, Attribute::NoCallback),
+// CHECK-NEXT: Attribute::get(C, Attribute::NoSync),
+// CHECK-NEXT: Attribute::get(C, Attribute::NoFree),
+// CHECK-NEXT: Attribute::get(C, Attribute::WillReturn),
+// CHECK-NEXT: // ArgMem: NoModRef, InaccessibleMem: NoModRef, ErrnoMem: NoModRef, AARCH64_FPMR: NoModRef, AARCH64_ZA: Ref, Other: NoModRef
+// CHECK-NEXT: Attribute::getWithMemoryEffects(C, MemoryEffects::createFromIntValue(256)),
+// CHECK-NEXT: });
+// CHECK-NEXT: case 2:
+// CHECK-NEXT: return AttributeSet::get(C, {
+// CHECK-NEXT: Attribute::get(C, Attribute::NoUnwind),
+// CHECK-NEXT: Attribute::get(C, Attribute::NoCallback),
+// CHECK-NEXT: Attribute::get(C, Attribute::NoSync),
+// CHECK-NEXT: Attribute::get(C, Attribute::NoFree),
+// CHECK-NEXT: Attribute::get(C, Attribute::WillReturn),
+// CHECK-NEXT: // ArgMem: NoModRef, InaccessibleMem: NoModRef, ErrnoMem: NoModRef, AARCH64_FPMR: Mod, AARCH64_ZA: NoModRef, Other: NoModRef
+// CHECK-NEXT: Attribute::getWithMemoryEffects(C, MemoryEffects::createFromIntValue(128)),
+
+// CHECK: static constexpr uint16_t IntrinsicsToAttributesMap[] = {
+// CHECK-NEXT: 0 << 8 | 0, // llvm.aarch64.get.fpmr.set.za
+// CHECK-NEXT: 1 << 8 | 0, // llvm.aarch64.get.za.2
+// CHECK-NEXT: 2 << 8 | 0, // llvm.aarch64.set.fpmr.2
+// CHECK-NEXT:};
diff --git a/llvm/unittests/Support/ModRefTest.cpp b/llvm/unittests/Support/ModRefTest.cpp
index 9c13908da44bb..7aa473ad20336 100644
--- a/llvm/unittests/Support/ModRefTest.cpp
+++ b/llvm/unittests/Support/ModRefTest.cpp
@@ -21,7 +21,8 @@ TEST(ModRefTest, PrintMemoryEffects) {
raw_string_ostream OS(S);
OS << MemoryEffects::none();
EXPECT_EQ(S, "ArgMem: NoModRef, InaccessibleMem: NoModRef, ErrnoMem: "
- "NoModRef, Other: NoModRef");
+ "NoModRef, AARCH64_FPMR: NoModRef, AARCH64_ZA: NoModRef, Other: "
+ "NoModRef");
}
} // namespace
diff --git a/llvm/utils/TableGen/Basic/CodeGenIntrinsics.cpp b/llvm/utils/TableGen/Basic/CodeGenIntrinsics.cpp
index bc42efa3b2e9c..eb2d4de7e9320 100644
--- a/llvm/utils/TableGen/Basic/CodeGenIntrinsics.cpp
+++ b/llvm/utils/TableGen/Basic/CodeGenIntrinsics.cpp
@@ -374,7 +374,19 @@ void CodeGenIntrinsic::setProperty(const Record *R) {
ME &= MemoryEffects::argMemOnly();
else if (R->getName() == "IntrInaccessibleMemOnly")
ME &= MemoryEffects::inaccessibleMemOnly();
- else if (R->getName() == "IntrInaccessibleMemOrArgMemOnly")
+ else if (R->isSubClassOf("IntrInaccessibleReadMemOnly")) {
+ llvm::IRMemLocation Loc = R->getLocationTypeAsInt("Loc");
+ if (ME.onlyAccessTargetMemoryLocation())
+ ME = ME.getWithModRef(Loc, ModRefInfo::Ref);
+ else
+ ME &= MemoryEffects::inaccessibleReadMemOnly(Loc);
+ } else if (R->isSubClassOf("IntrInaccessibleWriteMemOnly")) {
+ llvm::IRMemLocation Loc = R->getLocationTypeAsInt("Loc");
+ if (ME.onlyAccessTargetMemoryLocation())
+ ME = ME.getWithModRef(Loc, ModRefInfo::Mod);
+ else
+ ME &= MemoryEffects::inaccessibleWriteMemOnly(Loc);
+ } else if (R->getName() == "IntrInaccessibleMemOrArgMemOnly")
ME &= MemoryEffects::inaccessibleOrArgMemOnly();
else if (R->getName() == "Commutative")
isCommutative = true;
|
|
@llvm/pr-subscribers-llvm-transforms Author: None (CarolineConcatto) Changes…ocations This patch introduces preliminary support for additional memory locations, such as FPMR and ZA, needed to model AArch64 architectural registers as memory dependencies. It was a solution suggested in : https://discourse.llvm.org/t/rfc-improving-fpmr-handling-for-fp8-intrinsics-in-llvm/86868/6 Currently, these locations are not yet target-specific. The goal is to enable the compiler to express read/write effects on these resources. What This Patch Does: Current Limitations: This patch is not functionally complete — it is a structural prototype to solicit feedback on the direction and I would like some suggestion on how to proceed. Full diff: https://github.com/llvm/llvm-project/pull/148650.diff 15 Files Affected:
diff --git a/llvm/include/llvm/AsmParser/LLToken.h b/llvm/include/llvm/AsmParser/LLToken.h
index c7e4bdf3ff811..c08eb99c1f5b2 100644
--- a/llvm/include/llvm/AsmParser/LLToken.h
+++ b/llvm/include/llvm/AsmParser/LLToken.h
@@ -202,6 +202,8 @@ enum Kind {
kw_readwrite,
kw_argmem,
kw_inaccessiblemem,
+ kw_aarch64_fpmr,
+ kw_aarch64_za,
kw_errnomem,
// Legacy attributes:
diff --git a/llvm/include/llvm/IR/Intrinsics.td b/llvm/include/llvm/IR/Intrinsics.td
index bd6f94ac1286c..ad1b0b462be37 100644
--- a/llvm/include/llvm/IR/Intrinsics.td
+++ b/llvm/include/llvm/IR/Intrinsics.td
@@ -49,6 +49,17 @@ def IntrArgMemOnly : IntrinsicProperty;
// accessible by the module being compiled. This is a weaker form of IntrNoMem.
def IntrInaccessibleMemOnly : IntrinsicProperty;
+
+
+class IntrinsicMemoryLocation;
+// This should be added in the Target, but once in IntrinsicsAArch64.td
+// It complains error: "Variable not defined: 'AArch64_FPMR'"
+def AArch64_FPMR : IntrinsicMemoryLocation;
+def AArch64_ZA: IntrinsicMemoryLocation;
+// IntrInaccessible{Read|Write}MemOnly needs to set Location
+class IntrInaccessibleReadMemOnly<IntrinsicMemoryLocation idx> : IntrinsicProperty{IntrinsicMemoryLocation Loc=idx;}
+class IntrInaccessibleWriteMemOnly<IntrinsicMemoryLocation idx> : IntrinsicProperty{IntrinsicMemoryLocation Loc=idx;}
+
// IntrInaccessibleMemOrArgMemOnly -- This intrinsic only accesses memory that
// its pointer-typed arguments point to or memory that is not accessible
// by the module being compiled. This is a weaker form of IntrArgMemOnly.
diff --git a/llvm/include/llvm/Support/ModRef.h b/llvm/include/llvm/Support/ModRef.h
index 71f3b5bcb9c2b..0de2b02e4e05a 100644
--- a/llvm/include/llvm/Support/ModRef.h
+++ b/llvm/include/llvm/Support/ModRef.h
@@ -56,6 +56,11 @@ enum class ModRefInfo : uint8_t {
/// Debug print ModRefInfo.
LLVM_ABI raw_ostream &operator<<(raw_ostream &OS, ModRefInfo MR);
+enum class InaccessibleTargetMemLocation {
+ AARCH64_FPMR = 3,
+ AARCH64_ZA = 4,
+};
+
/// The locations at which a function might access memory.
enum class IRMemLocation {
/// Access to memory via argument pointers.
@@ -65,7 +70,7 @@ enum class IRMemLocation {
/// Errno memory.
ErrnoMem = 2,
/// Any other memory.
- Other = 3,
+ Other = 5,
/// Helpers to iterate all locations in the MemoryEffectsBase class.
First = ArgMem,
@@ -152,6 +157,40 @@ template <typename LocationEnum> class MemoryEffectsBase {
return MemoryEffectsBase(Location::Other, MR);
}
+ /// Create MemoryEffectsBase that can only read inaccessible memory.
+ static MemoryEffectsBase
+ inaccessibleReadMemOnly(Location Loc = Location::InaccessibleMem) {
+ return MemoryEffectsBase(Loc, ModRefInfo::Ref);
+ }
+
+ /// Create MemoryEffectsBase that can only write inaccessible memory.
+ static MemoryEffectsBase
+ inaccessibleWriteMemOnly(Location Loc = Location::InaccessibleMem) {
+ return MemoryEffectsBase(Loc, ModRefInfo::Mod);
+ }
+
+ /// Checks if only target-specific memory locations are set.
+ /// Ignores standard locations like ArgMem or InaccessibleMem.
+ /// Needed because `Data` may be non-zero by default unless explicitly
+ /// cleared.
+ bool onlyAccessTargetMemoryLocation() {
+ MemoryEffectsBase ME = *this;
+ for (unsigned I = static_cast<int>(LocationEnum::ErrnoMem);
+ I < static_cast<int>(LocationEnum::Last); I++)
+ ME = ME.getWithoutLoc(static_cast<IRMemLocation>(I));
+ return ME.doesNotAccessMemory();
+ }
+
+ /// Create MemoryEffectsBase that can only access Target Memory Locations
+ static MemoryEffectsBase
+ setTargetMemLocationModRef(ModRefInfo MR = ModRefInfo::NoModRef) {
+ MemoryEffectsBase FRMB = none();
+ for (unsigned I = static_cast<int>(LocationEnum::ErrnoMem);
+ I < static_cast<int>(LocationEnum::Last); I++)
+ FRMB.setModRef(static_cast<Location>(I), MR);
+ return FRMB;
+ }
+
/// Create MemoryEffectsBase that can only access inaccessible or argument
/// memory.
static MemoryEffectsBase
@@ -178,6 +217,11 @@ template <typename LocationEnum> class MemoryEffectsBase {
return MemoryEffectsBase(Data);
}
+ bool isTargetMemLoc(IRMemLocation Loc) {
+ return static_cast<unsigned>(Loc) >
+ static_cast<unsigned>(Location::ErrnoMem);
+ }
+
/// Convert MemoryEffectsBase into an encoded integer value (used by memory
/// attribute).
uint32_t toIntValue() const {
diff --git a/llvm/include/llvm/TableGen/Record.h b/llvm/include/llvm/TableGen/Record.h
index a2b86eb8e7cad..5aeb331c49c9b 100644
--- a/llvm/include/llvm/TableGen/Record.h
+++ b/llvm/include/llvm/TableGen/Record.h
@@ -25,6 +25,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/ModRef.h"
#include "llvm/Support/SMLoc.h"
#include "llvm/Support/Timer.h"
#include "llvm/Support/TrailingObjects.h"
@@ -1961,6 +1962,8 @@ class Record {
/// value is not the right type.
int64_t getValueAsInt(StringRef FieldName) const;
+ llvm::IRMemLocation getLocationTypeAsInt(StringRef FieldName) const;
+
/// This method looks up the specified field and returns its value as an Dag,
/// throwing an exception if the field does not exist or if the value is not
/// the right type.
diff --git a/llvm/lib/AsmParser/LLLexer.cpp b/llvm/lib/AsmParser/LLLexer.cpp
index ce813e1d7b1c4..c086f9f9585a2 100644
--- a/llvm/lib/AsmParser/LLLexer.cpp
+++ b/llvm/lib/AsmParser/LLLexer.cpp
@@ -701,6 +701,8 @@ lltok::Kind LLLexer::LexIdentifier() {
KEYWORD(write);
KEYWORD(readwrite);
KEYWORD(argmem);
+ KEYWORD(aarch64_fpmr);
+ KEYWORD(aarch64_za);
KEYWORD(inaccessiblemem);
KEYWORD(errnomem);
KEYWORD(argmemonly);
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index b7f6950f679ef..abde2993bb048 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -1666,6 +1666,25 @@ static bool upgradeMemoryAttr(MemoryEffects &ME, lltok::Kind Kind) {
}
}
+static std::optional<MemoryEffects::Location> keywordToLoc(lltok::Kind Tok) {
+ switch (Tok) {
+ case lltok::kw_argmem:
+ return IRMemLocation::ArgMem;
+ case lltok::kw_inaccessiblemem:
+ return IRMemLocation::InaccessibleMem;
+ case lltok::kw_errnomem:
+ return IRMemLocation::ErrnoMem;
+ case lltok::kw_aarch64_fpmr:
+ return static_cast<IRMemLocation>(
+ llvm::InaccessibleTargetMemLocation::AARCH64_FPMR);
+ case lltok::kw_aarch64_za:
+ return static_cast<IRMemLocation>(
+ llvm::InaccessibleTargetMemLocation::AARCH64_ZA);
+ default:
+ return std::nullopt;
+ }
+}
+
/// parseFnAttributeValuePairs
/// ::= <attr> | <attr> '=' <value>
bool LLParser::parseFnAttributeValuePairs(AttrBuilder &B,
@@ -2510,19 +2529,6 @@ bool LLParser::parseAllocKind(AllocFnKind &Kind) {
return false;
}
-static std::optional<MemoryEffects::Location> keywordToLoc(lltok::Kind Tok) {
- switch (Tok) {
- case lltok::kw_argmem:
- return IRMemLocation::ArgMem;
- case lltok::kw_inaccessiblemem:
- return IRMemLocation::InaccessibleMem;
- case lltok::kw_errnomem:
- return IRMemLocation::ErrnoMem;
- default:
- return std::nullopt;
- }
-}
-
static std::optional<ModRefInfo> keywordToModRef(lltok::Kind Tok) {
switch (Tok) {
case lltok::kw_none:
diff --git a/llvm/lib/IR/Attributes.cpp b/llvm/lib/IR/Attributes.cpp
index d1fbcb9e893a7..37e9d7c5c74db 100644
--- a/llvm/lib/IR/Attributes.cpp
+++ b/llvm/lib/IR/Attributes.cpp
@@ -640,6 +640,10 @@ std::string Attribute::getAsString(bool InAttrGrp) const {
if (MR == OtherMR)
continue;
+ // Dont want to print Target Location if NoModRef
+ if (ME.isTargetMemLoc(Loc) && (MR == ModRefInfo::NoModRef))
+ continue;
+
if (!First)
OS << ", ";
First = false;
@@ -656,6 +660,15 @@ std::string Attribute::getAsString(bool InAttrGrp) const {
break;
case IRMemLocation::Other:
llvm_unreachable("This is represented as the default access kind");
+ default: {
+ InaccessibleTargetMemLocation TargetLoc =
+ static_cast<InaccessibleTargetMemLocation>(Loc);
+ if (TargetLoc == InaccessibleTargetMemLocation::AARCH64_FPMR)
+ OS << "aarch64_fpmr: ";
+ if (TargetLoc == InaccessibleTargetMemLocation::AARCH64_ZA)
+ OS << "aarch64_za: ";
+ break;
+ }
}
OS << getModRefStr(MR);
}
diff --git a/llvm/lib/Support/ModRef.cpp b/llvm/lib/Support/ModRef.cpp
index 2bb9bc945bd2e..dc0dafdbe7e49 100644
--- a/llvm/lib/Support/ModRef.cpp
+++ b/llvm/lib/Support/ModRef.cpp
@@ -49,6 +49,15 @@ raw_ostream &llvm::operator<<(raw_ostream &OS, MemoryEffects ME) {
case IRMemLocation::Other:
OS << "Other: ";
break;
+ default: {
+ InaccessibleTargetMemLocation TargetLoc =
+ static_cast<InaccessibleTargetMemLocation>(Loc);
+ if (TargetLoc == InaccessibleTargetMemLocation::AARCH64_FPMR)
+ OS << "AARCH64_FPMR: ";
+ if (TargetLoc == InaccessibleTargetMemLocation::AARCH64_ZA)
+ OS << "AARCH64_ZA: ";
+ break;
+ }
}
OS << ME.getModRef(Loc);
});
diff --git a/llvm/lib/TableGen/Record.cpp b/llvm/lib/TableGen/Record.cpp
index 1f3e5dc68f1d6..d114358266737 100644
--- a/llvm/lib/TableGen/Record.cpp
+++ b/llvm/lib/TableGen/Record.cpp
@@ -3102,6 +3102,21 @@ Record::getValueAsListOfDefs(StringRef FieldName) const {
return Defs;
}
+llvm::IRMemLocation Record::getLocationTypeAsInt(StringRef FieldName) const {
+ const Record *LocRec = getValueAsDef(FieldName);
+ StringRef Name = LocRec->getName();
+ if (Name == "AArch64_FPMR")
+ return static_cast<IRMemLocation>(
+ llvm::InaccessibleTargetMemLocation::AARCH64_FPMR);
+ else if (Name == "AArch64_ZA")
+ return static_cast<IRMemLocation>(
+ llvm::InaccessibleTargetMemLocation::AARCH64_ZA);
+ else if (Name == "InaccessibleMem")
+ return llvm::IRMemLocation::InaccessibleMem;
+ else
+ PrintFatalError(getLoc(), "unknown IRMemLocation: " + Name);
+}
+
int64_t Record::getValueAsInt(StringRef FieldName) const {
const RecordVal *R = getValue(FieldName);
if (!R || !R->getValue())
diff --git a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
index f43202eea6306..49b822b3ef38e 100644
--- a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
@@ -143,6 +143,9 @@ static void addLocAccess(MemoryEffects &ME, const MemoryLocation &Loc,
ME |= MemoryEffects::argMemOnly(MR);
ME |= MemoryEffects(IRMemLocation::ErrnoMem, MR);
ME |= MemoryEffects(IRMemLocation::Other, MR);
+ // Should also set the other Target Memory Locations as MR.
+ // To compares with MemoryEffects::unknown() in addMemoryAttrs
+ ME |= MemoryEffects::setTargetMemLocationModRef(MR);
}
static void addArgLocs(MemoryEffects &ME, const CallBase *Call,
diff --git a/llvm/test/Assembler/memory-attribute.ll b/llvm/test/Assembler/memory-attribute.ll
index effd4ce7c4548..42f9b9f87e8b0 100644
--- a/llvm/test/Assembler/memory-attribute.ll
+++ b/llvm/test/Assembler/memory-attribute.ll
@@ -78,3 +78,28 @@ declare void @fn_argmem_read_inaccessiblemem_write()
; CHECK: @fn_argmem_read_inaccessiblemem_write_reordered()
declare void @fn_argmem_read_inaccessiblemem_write_reordered()
memory(inaccessiblemem: write, argmem: read)
+
+; CHECK: Function Attrs: memory(aarch64_za: write)
+; CHECK: @fn_inaccessiblemem_write_aarch64_za()
+declare void @fn_inaccessiblemem_write_aarch64_za()
+ memory(aarch64_za: write)
+
+; CHECK: Function Attrs: memory(aarch64_za: read)
+; CHECK: @fn_inaccessiblemem_read_aarch64_za()
+declare void @fn_inaccessiblemem_read_aarch64_za()
+ memory(aarch64_za: read)
+
+; CHECK: Function Attrs: memory(aarch64_fpmr: write)
+; CHECK: @fn_inaccessiblemem_write_aarch64_fpmr()
+declare void @fn_inaccessiblemem_write_aarch64_fpmr()
+ memory(aarch64_fpmr: write)
+
+; CHECK: Function Attrs: memory(aarch64_fpmr: read)
+; CHECK: @fn_inaccessiblemem_read_aarch64_fpmr()
+declare void @fn_inaccessiblemem_read_aarch64_fpmr()
+ memory(aarch64_fpmr: read)
+
+; CHECK: Function Attrs: memory(aarch64_fpmr: read, aarch64_za: write)
+; CHECK: @fn_inaccessiblemem_read_aarch64_fpmr_write_aarch64_za()
+declare void @fn_inaccessiblemem_read_aarch64_fpmr_write_aarch64_za()
+ memory(aarch64_fpmr: read, aarch64_za: write)
diff --git a/llvm/test/Bitcode/attributes.ll b/llvm/test/Bitcode/attributes.ll
index 8c1a76365e1b4..8e72e7ade54c1 100644
--- a/llvm/test/Bitcode/attributes.ll
+++ b/llvm/test/Bitcode/attributes.ll
@@ -572,7 +572,6 @@ define void @dead_on_return(ptr dead_on_return %p) {
ret void
}
-; CHECK: attributes #0 = { noreturn }
; CHECK: attributes #1 = { nounwind }
; CHECK: attributes #2 = { memory(none) }
; CHECK: attributes #3 = { memory(read) }
diff --git a/llvm/test/TableGen/intrinsic-attrs-fp8.td b/llvm/test/TableGen/intrinsic-attrs-fp8.td
new file mode 100644
index 0000000000000..5aaba44edcc45
--- /dev/null
+++ b/llvm/test/TableGen/intrinsic-attrs-fp8.td
@@ -0,0 +1,54 @@
+// RUN: llvm-tblgen -gen-intrinsic-impl -I %p/../../include -DTEST_INTRINSICS_SUPPRESS_DEFS %s | FileCheck %s
+
+include "llvm/IR/Intrinsics.td"
+
+def int_aarch64_set_fpmr_2 : DefaultAttrsIntrinsic<[], [llvm_i64_ty], [IntrInaccessibleWriteMemOnly<AArch64_FPMR>]>;
+
+def int_aarch64_get_za_2 : DefaultAttrsIntrinsic<[], [llvm_i64_ty], [IntrInaccessibleReadMemOnly<AArch64_ZA>]>;
+
+def int_aarch64_get_fpmr_set_za : DefaultAttrsIntrinsic<[], [llvm_i64_ty], [IntrInaccessibleReadMemOnly<AArch64_FPMR>, IntrInaccessibleWriteMemOnly<AArch64_ZA>]>;
+
+// CHECK: static constexpr unsigned IntrinsicNameOffsetTable[] = {
+// CHECK-NEXT: 1, // not_intrinsic
+// CHECK-NEXT: 15, // llvm.aarch64.get.fpmr.set.za
+// CHECK-NEXT: 44, // llvm.aarch64.get.za.2
+// CHECK-NEXT: 66, // llvm.aarch64.set.fpmr.2
+
+// CHECK: static AttributeSet getIntrinsicFnAttributeSet(LLVMContext &C, unsigned ID) {
+// CHECK-NEXT: switch (ID) {
+// CHECK-NEXT: default: llvm_unreachable("Invalid attribute set number");
+// CHECK-NEXT: case 0:
+// CHECK-NEXT: return AttributeSet::get(C, {
+// CHECK-NEXT: Attribute::get(C, Attribute::NoUnwind),
+// CHECK-NEXT: Attribute::get(C, Attribute::NoCallback),
+// CHECK-NEXT: Attribute::get(C, Attribute::NoSync),
+// CHECK-NEXT: Attribute::get(C, Attribute::NoFree),
+// CHECK-NEXT: Attribute::get(C, Attribute::WillReturn),
+// CHECK-NEXT: // ArgMem: NoModRef, InaccessibleMem: NoModRef, ErrnoMem: NoModRef, AARCH64_FPMR: Ref, AARCH64_ZA: Mod, Other: NoModRef
+// CHECK-NEXT: Attribute::getWithMemoryEffects(C, MemoryEffects::createFromIntValue(576)),
+// CHECK-NEXT: });
+// CHECK-NEXT: case 1:
+// CHECK-NEXT: return AttributeSet::get(C, {
+// CHECK-NEXT: Attribute::get(C, Attribute::NoUnwind),
+// CHECK-NEXT: Attribute::get(C, Attribute::NoCallback),
+// CHECK-NEXT: Attribute::get(C, Attribute::NoSync),
+// CHECK-NEXT: Attribute::get(C, Attribute::NoFree),
+// CHECK-NEXT: Attribute::get(C, Attribute::WillReturn),
+// CHECK-NEXT: // ArgMem: NoModRef, InaccessibleMem: NoModRef, ErrnoMem: NoModRef, AARCH64_FPMR: NoModRef, AARCH64_ZA: Ref, Other: NoModRef
+// CHECK-NEXT: Attribute::getWithMemoryEffects(C, MemoryEffects::createFromIntValue(256)),
+// CHECK-NEXT: });
+// CHECK-NEXT: case 2:
+// CHECK-NEXT: return AttributeSet::get(C, {
+// CHECK-NEXT: Attribute::get(C, Attribute::NoUnwind),
+// CHECK-NEXT: Attribute::get(C, Attribute::NoCallback),
+// CHECK-NEXT: Attribute::get(C, Attribute::NoSync),
+// CHECK-NEXT: Attribute::get(C, Attribute::NoFree),
+// CHECK-NEXT: Attribute::get(C, Attribute::WillReturn),
+// CHECK-NEXT: // ArgMem: NoModRef, InaccessibleMem: NoModRef, ErrnoMem: NoModRef, AARCH64_FPMR: Mod, AARCH64_ZA: NoModRef, Other: NoModRef
+// CHECK-NEXT: Attribute::getWithMemoryEffects(C, MemoryEffects::createFromIntValue(128)),
+
+// CHECK: static constexpr uint16_t IntrinsicsToAttributesMap[] = {
+// CHECK-NEXT: 0 << 8 | 0, // llvm.aarch64.get.fpmr.set.za
+// CHECK-NEXT: 1 << 8 | 0, // llvm.aarch64.get.za.2
+// CHECK-NEXT: 2 << 8 | 0, // llvm.aarch64.set.fpmr.2
+// CHECK-NEXT:};
diff --git a/llvm/unittests/Support/ModRefTest.cpp b/llvm/unittests/Support/ModRefTest.cpp
index 9c13908da44bb..7aa473ad20336 100644
--- a/llvm/unittests/Support/ModRefTest.cpp
+++ b/llvm/unittests/Support/ModRefTest.cpp
@@ -21,7 +21,8 @@ TEST(ModRefTest, PrintMemoryEffects) {
raw_string_ostream OS(S);
OS << MemoryEffects::none();
EXPECT_EQ(S, "ArgMem: NoModRef, InaccessibleMem: NoModRef, ErrnoMem: "
- "NoModRef, Other: NoModRef");
+ "NoModRef, AARCH64_FPMR: NoModRef, AARCH64_ZA: NoModRef, Other: "
+ "NoModRef");
}
} // namespace
diff --git a/llvm/utils/TableGen/Basic/CodeGenIntrinsics.cpp b/llvm/utils/TableGen/Basic/CodeGenIntrinsics.cpp
index bc42efa3b2e9c..eb2d4de7e9320 100644
--- a/llvm/utils/TableGen/Basic/CodeGenIntrinsics.cpp
+++ b/llvm/utils/TableGen/Basic/CodeGenIntrinsics.cpp
@@ -374,7 +374,19 @@ void CodeGenIntrinsic::setProperty(const Record *R) {
ME &= MemoryEffects::argMemOnly();
else if (R->getName() == "IntrInaccessibleMemOnly")
ME &= MemoryEffects::inaccessibleMemOnly();
- else if (R->getName() == "IntrInaccessibleMemOrArgMemOnly")
+ else if (R->isSubClassOf("IntrInaccessibleReadMemOnly")) {
+ llvm::IRMemLocation Loc = R->getLocationTypeAsInt("Loc");
+ if (ME.onlyAccessTargetMemoryLocation())
+ ME = ME.getWithModRef(Loc, ModRefInfo::Ref);
+ else
+ ME &= MemoryEffects::inaccessibleReadMemOnly(Loc);
+ } else if (R->isSubClassOf("IntrInaccessibleWriteMemOnly")) {
+ llvm::IRMemLocation Loc = R->getLocationTypeAsInt("Loc");
+ if (ME.onlyAccessTargetMemoryLocation())
+ ME = ME.getWithModRef(Loc, ModRefInfo::Mod);
+ else
+ ME &= MemoryEffects::inaccessibleWriteMemOnly(Loc);
+ } else if (R->getName() == "IntrInaccessibleMemOrArgMemOnly")
ME &= MemoryEffects::inaccessibleOrArgMemOnly();
else if (R->getName() == "Commutative")
isCommutative = true;
|
CarolineConcatto
changed the title
|
I am removing from draft, but we are still open to suggestions. Maybe removing from draft to get more attention |
| kw_readwrite, | ||
| kw_argmem, | ||
| kw_inaccessiblemem, | ||
| kw_aarch64_fpmr, |
There was a problem hiding this comment.
I'll let @nikic comment, but are we really planning to extend LLVM IR syntax with arch specific keywords?
There was a problem hiding this comment.
Is this new? There are many examples mainly linked to calling conventions and function/argument attributes.
jurahul
changed the title
We could easily increase the size to 64 bits, which gives us 32 locations. My primary concern with making this dependent on the backend is that I don't want to thread backend information through to every place that works with MemoryEffects. However, I guess that generic code doesn't actually care about what a target-specific memory location refers to. So a possible approach would be to just reserve a fixed number of target-memory locations like Does this sound reasonable to you?
That would make sense to me. |
I think one of the advantage of having out-of-line target-memory locations instead of a fixed target1..targetN bitset is that we could print them friendlier, e.g., |
|
Hi @nikic , The printing of memory(target1: /aarch64.fpmr/ read) is happening in the file Attributes.cpp and that is the part I may lack understanding of how to link this with the target architecture. I will need to add a Target hook in that part of the code, but atm there is not information about the target. |
But how would you represent this without making MemoryEffects an expensive data structure? |
Right, that's the challenge with making any of this target-dependent. The best idea I have to do this without a lot of changes would be to optionally pass down the Triple to |
|
Hi @nikic, |
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions h,cpp -- llvm/include/llvm/AsmParser/LLToken.h llvm/include/llvm/Support/ModRef.h llvm/lib/AsmParser/LLLexer.cpp llvm/lib/AsmParser/LLParser.cpp llvm/lib/IR/AsmWriter.cpp llvm/lib/IR/Attributes.cpp llvm/lib/Support/ModRef.cpp llvm/lib/Transforms/IPO/FunctionAttrs.cpp llvm/unittests/Support/ModRefTest.cpp llvm/utils/TableGen/Basic/CodeGenIntrinsics.cpp llvm/utils/TableGen/Basic/CodeGenIntrinsics.h
View the diff from clang-format here.diff --git a/llvm/include/llvm/Support/ModRef.h b/llvm/include/llvm/Support/ModRef.h
index b15d4f778..b0a7d67c5 100644
--- a/llvm/include/llvm/Support/ModRef.h
+++ b/llvm/include/llvm/Support/ModRef.h
@@ -176,7 +176,7 @@ public:
bool onlyAccessTargetMemoryLocation() {
MemoryEffectsBase ME = *this;
for (unsigned I = static_cast<int>(LocationEnum::FirstTarget);
- I <= static_cast<int>(LocationEnum::Last); I++)
+ I <= static_cast<int>(LocationEnum::Last); I++)
ME = ME.getWithoutLoc(static_cast<IRMemLocation>(I));
return ME.doesNotAccessMemory();
}
diff --git a/llvm/lib/IR/AsmWriter.cpp b/llvm/lib/IR/AsmWriter.cpp
index 2b3d089b6..ff96da243 100644
--- a/llvm/lib/IR/AsmWriter.cpp
+++ b/llvm/lib/IR/AsmWriter.cpp
@@ -4981,8 +4981,8 @@ void AssemblyWriter::writeAllAttributeGroups() {
asVec[I.second] = I;
for (const auto &I : asVec)
- Out << "attributes #" << I.second << " = { "
- << I.first.getAsString(true) << " }\n";
+ Out << "attributes #" << I.second << " = { " << I.first.getAsString(true)
+ << " }\n";
}
void AssemblyWriter::printUseListOrder(const Value *V,
diff --git a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
index 2c42ed781..8377104aa 100644
--- a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
@@ -145,7 +145,8 @@ static void addLocAccess(MemoryEffects &ME, const MemoryLocation &Loc,
// Should also set the other Target Memory Locations as MR.
// To compares with MemoryEffects::unknown() in addMemoryAttrs
ME |= MemoryEffects(IRMemLocation::TargetMem0, MR);
- ME |= MemoryEffects(IRMemLocation::TargetMem1, MR);;
+ ME |= MemoryEffects(IRMemLocation::TargetMem1, MR);
+ ;
}
static void addArgLocs(MemoryEffects &ME, const CallBase *Call,
|
CarolineConcatto
changed the title
|
@nikic gentle ping! |
|
I've taken a look over the PR and think it's worth breaking in two. It looks like we've settled on a design whereby there are a set of InaccessibleMem locations that are reserved for use by target specific operations only. Then on top of this we have an optional target specific veneer to enable pretty parsing and printing of these to better fit their use within the target's domain. It would be better to have separate PRs to show this (i.e. one to add the generic TargetMem# locations, and another to add the AArch64 veneer to parse/print "aarch64_fpmr/za". To shorten the naming I'm wondering if we can settle on TargetMem/target_mem and then implicitly assume (well document in the langref) these to be variants of inaccessible memory, albeit ones that are specifically independent to InaccessibleMem, which is the location to use for non-target-specific or non-named-target-specific variants of inaccessible memory. |
…ocations This patch introduces preliminary support for additional memory locations, such as FPMR and ZA, needed to model AArch64 architectural registers as memory dependencies. Currently, these locations are not yet target-specific. The goal is to enable the compiler to express read/write effects on these resources. What This Patch Does: Adds two new memory locations: FPMR and ZA, intended to represent AArch64-specific inaccessible memory types. Current Limitations: These new locations are not yet target-specific in the type-safe sense, they are globally visible and hardcoded. There is no mechanism yet to associate a memory location with its corresponding target (e.g., AArch64 vs RISCV). No changes are made yet to bitcode serialization, parser support, or alias analysis behavior. This patch is not functionally complete — it is a structural prototype to solicit feedback on the direction and I would like some suggestion on how to proceed.
This patch is removing the keyworkd and token inaccessibleWrite and inaccessibleRead. It is using Read and Write to set the Target Memory Location. Adding "aarch64" in front of the target specific memory locations
This patch replaces target‑specific memory location names in IR printing/parsing with generic target memory kinds whose meaning is determined by the module’s target triple. Replace the target specific names by generic target memory kinds (e.g. mem_target_0, mem_target_1), and make their user‑facing names triple‑dependent. Plumb the Triple into attribute so textual IR reflects the correct per‑target names. To print a meaningful name each target needs to add a keyword->kind mapping so textual forms remain readable and round‑trip correctly.
CarolineConcatto
force-pushed
the
fpmr_metadata
branch
from
c39184f to
e8fc7eb
Compare
|
Hi @paulwalker-arm, |
| kw_aarch64_fpmr, | ||
| kw_aarch64_za, |
There was a problem hiding this comment.
I had in mind that these would be part of the follow-on PR as well? The idea being this PR keeps everything generic and fully supports target_mem# (i.e. both printing and parsing).
There was a problem hiding this comment.
We need a token to be used when describing the memory:
memory(aarch64_za : read)
There are 3 solutions:
-
Create a target specific token for each architecture(arch64_za), but without a generic token(target_mem0). It is the one I chosen because each target needs to describe its token to be able to use the target location . All tokens should have the architecture name, like suggested. So it could not borrow from other architectures.
So this option is allowed:
memory(aarch64_za : read)
but does not allow:
memory(target_mem0 : read) -
Create a generic token and print what is this meaning when we add target triple support, but do not add a target specific token. There would be the tokens target_memX and when we add the triple support it could print whatever the target wants. So only the Triple.h would know the name of the target locations, not even in the description of the memory we could use a target specific name.
So this option is allowed:
memory(target_mem0 : read)
but does not allow:
memory(aarch64_za : read) -
Could add target specific and generic tokens. I did not liked this option because it may cause confusion being able to describe the same thing in two ways so thing could get messy.
So these options are allowed:
memory(aarch64_za : read)
and
memory(target_mem0 : read)
Do you have a preferred solution? Or do you see another way of doing this?
There was a problem hiding this comment.
Based on the conversation about not wanting to thread backend information throughout the MemoryEffect code I think option 3 (with all AArch64 specific code in a separate PR) is the only via option, because otherwise we run the risk of LLVM not being able to parse it's own output.
It also fix this warning: ModRef.cpp:52:5: error: default label in switch which covers all enumeration values [-Werror,-Wcovered-switch-default]
llvm/include/llvm/Support/ModRef.h
Outdated
| enum class InaccessibleTargetMemLocation { | ||
| TargetMem0 = 3, | ||
| TargetMem1 = 4, | ||
| }; |
There was a problem hiding this comment.
Is there a good reason to separate these from IRMemLocation? Parts of the patch is forced to use >ErrnoMem because there's no enum value to directly compare. Having all the locations in IRMemLocation removes this problem along with a bunch on unnecessary casting.
When doing this perhaps it's worth adding FirstTargetMem = TargetMem0 and LastTargetMem = TargetMem1 then using this for range checking, that way it'll be easy to add more TargetMem locations in the future.
There was a problem hiding this comment.
So my main concern was to have some distinction between "Generic" Memory Location("ArgMem, Inaccessible, Error and Other") from Target Memory Locations. By having two more descriptors besides first and last may also help. I will implement that.
llvm/include/llvm/Support/ModRef.h
Outdated
| return static_cast<unsigned>(Loc) > | ||
| static_cast<unsigned>(Location::ErrnoMem); |
There was a problem hiding this comment.
Location::Other is after Location::ErrnoMem but is not a TargetMem location. This backs up my FirstTargetMem/LastTargetMem suggestion.
llvm/include/llvm/TableGen/Record.h
Outdated
| #include "llvm/ADT/StringRef.h" | ||
| #include "llvm/Support/Casting.h" | ||
| #include "llvm/Support/ErrorHandling.h" | ||
| #include "llvm/Support/ModRef.h" |
There was a problem hiding this comment.
Does this header specifically need ModRef.h?
| static std::optional<MemoryEffects::Location> keywordToLoc(lltok::Kind Tok) { | ||
| switch (Tok) { |
There was a problem hiding this comment.
What's the reason for moving the function?
llvm/lib/IR/AsmWriter.cpp
Outdated
| Out << "attributes #" << I.second << " = { " << I.first.getAsString(true) | ||
| << " }\n"; |
There was a problem hiding this comment.
I guess this is the result of clang-format for when the file had more extensive changes? which can now be reverted?
| // Should also set the other Target Memory Locations as MR. | ||
| // To compares with MemoryEffects::unknown() in addMemoryAttrs | ||
| ME |= MemoryEffects::setTargetMemLocationModRef(MR); |
There was a problem hiding this comment.
This look suspicious. The comment say's "it might be an argument" and passing "inaccessible memory" as an argument doesn't seem plausible. Looking at the two call sites I see one in addArgLocs which wouldn't be relevant based on the above and then checkFunctionMemoryAccess which already has dedicate handling for call instructions that touch inaccessible memory:
// Merge callee's memory effects into caller's ones, including
// inaccessible and errno memory, but excluding argument memory, which is
// handled separately.
ME |= CallME.getWithoutLoc(IRMemLocation::ArgMem);
There was a problem hiding this comment.
I am not sure I understand what is the problem here.
I've changed to be the same as IRMemLocation::Other and IRMemLocation::ErrnoMem.
| ; CHECK: Function Attrs: memory(target_mem1: write) | ||
| ; CHECK: @fn_inaccessiblemem_write_mem_target1() | ||
| declare void @fn_inaccessiblemem_write_mem_target1() | ||
| memory(target_mem1: write) | ||
|
|
||
| ; CHECK: Function Attrs: memory(target_mem1: read) | ||
| ; CHECK: @fn_inaccessiblemem_read_mem_target1() | ||
| declare void @fn_inaccessiblemem_read_mem_target1() | ||
| memory(target_mem1: read) |
There was a problem hiding this comment.
For my OCD brain, can the mem1 tests come after the mem0 ones :)
| target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128" | ||
| target triple = "aarch64" | ||
|
|
||
| ; CHECK: Function Attrs: memory(target_mem1: write) | ||
| ; CHECK: @fn_inaccessiblemem_write_target_mem1() [[ATTR0:#.*]] | ||
| declare void @fn_inaccessiblemem_write_target_mem1() | ||
| memory(target_mem1: write) | ||
|
|
||
| ; CHECK: Function Attrs: memory(target_mem1: read) | ||
| ; CHECK: @fn_inaccessiblemem_read_target_mem1() [[ATTR1:#.*]] | ||
| declare void @fn_inaccessiblemem_read_target_mem1() | ||
| memory(target_mem1: read) | ||
|
|
There was a problem hiding this comment.
Does this PR need an AArch64 specific test now that everything is generic?
llvm/lib/IR/Attributes.cpp
Outdated
| // Target Memory locations are not IRMemLocation. | ||
| // It is breaking buildbots when it treats warning as error | ||
| if (static_cast<int>(Loc) > static_cast<int>(IRMemLocation::ErrnoMem)) { |
There was a problem hiding this comment.
Presumably this all disappears once InaccessibleTargetMemLocation is removed and you can just update the switch (Loc) block directly.
| if (ME.isTargetMemLoc(Loc) && (MR == ModRefInfo::NoModRef)) | ||
| continue; |
There was a problem hiding this comment.
I don't understand why this is needed. Can you elaborate?
There was a problem hiding this comment.
Do we want to print the targetmem status even when it is NoModRef
If we remove this all the tests will need to be updated to print targetmem as well.
So this:
attributes #0 = { nounwind memory(readwrite, inaccessiblemem: none) }
will become this:
attributes #0 = { nounwind memory(readwrite, inaccessiblemem: none, target_mem0: none, target_mem1: none) }
But strange that it is not always. Only in some cases. This happens when :
MR that is the ModRef is the location ModRefInfo MR = ME.getModRef(Loc); is different than Other. Because other is the default.
// Print access kind for "other" as the default access kind.
if (MR == OtherMR)
CarolineConcatto
force-pushed
the
fpmr_metadata
branch
from
cc83871 to
189d9f7
Compare
This patch introduces preliminary support for additional memory locations, such as FPMR and ZA, needed to model AArch64 architectural registers as memory dependencies.
It was a solution suggested in : https://discourse.llvm.org/t/rfc-improving-fpmr-handling-for-fp8-intrinsics-in-llvm/86868/6
Currently, these locations are not yet target-specific. The goal is to enable the compiler to express read/write effects on these resources.
What This Patch Does:
Adds two new memory locations: FPMR and ZA, intended to represent
AArch64-specific inaccessible memory types.
Current Limitations:
These new locations are not yet target-specific in the type-safe sense,
they are globally visible and hardcoded.
There is no mechanism yet to associate a memory location with its
corresponding target (e.g., AArch64 vs RISCV).
No changes are made yet to bitcode serialization, parser support, or alias
analysis behavior.
This patch is not functionally complete — it is a structural prototype to solicit feedback on the direction and I would like some suggestion on how to proceed.